GLASGOW_CLASS_5_OMER_ALI_HTML_CSS_WEEK_2#206
Conversation
bonboh
left a comment
bonboh
left a comment
There was a problem hiding this comment.
Good job @Omer249a! One thing to note is that it's generally not a good idea to add set widths to elements, because it makes elements an exact width and not responsive. This is mainly why on a mobile screen the webpage doesn't fit properly and has got a horizontal scrollbar.
I've left a few comments and tips for you to read, but otherwise you don't need to do anything 👍
| body { | ||
| font-family: "Roboto", sans-serif; | ||
| -webkit-font-smoothing: antialiased; | ||
| font-size: 16px; |
There was a problem hiding this comment.
| font-size: 16px; |
We should never override the browser font size setting, as this means if the user changes their browser setting font size to 24px then this website doesn't increase in size. Also, it's good practice to use relative units like % or rem for font-size instead of px so elements change proportionally when the user changes font size.
| nav ul { | ||
| display: flex; | ||
| justify-content: space-evenly; | ||
| list-style: none; | ||
| width: 34rem; | ||
| } | ||
|
|
||
| nav ul a { | ||
| text-decoration: none; | ||
| color: #9c9fa7; | ||
| } |
There was a problem hiding this comment.
| nav ul { | |
| display: flex; | |
| justify-content: space-evenly; | |
| list-style: none; | |
| width: 34rem; | |
| } | |
| nav ul a { | |
| text-decoration: none; | |
| color: #9c9fa7; | |
| } | |
| nav ul { | |
| display: flex; | |
| justify-content: space-evenly; | |
| list-style: none; | |
| } | |
| nav ul a { | |
| text-decoration: none; | |
| color: #9c9fa7; | |
| padding: 1rem; | |
| } |
It's usually not a good idea to give an element a set width, because it makes it less flexible. Instead, we can remove the width and add padding to the a elements to add space between the links instead
| color: #9c9fa7; | ||
| } | ||
|
|
||
| nav ul a:hover { |
There was a problem hiding this comment.
| nav ul a:hover { | |
| nav ul a:focus, | |
| nav ul a:hover { |
When adding hover, we should also add focus so that keyboard users get the same styling when using the Tab key to jump to the link
| .hero h1, | ||
| h2 { |
There was a problem hiding this comment.
| .hero h1, | |
| h2 { | |
| .hero h1, | |
| .hero h2 { |
The second selector h2 will apply the style to all h2 elements on the page. We only want to update the h2 element in the hero section, so we should update the selector to .hero h2
| <!-- All the images you need are in the 'img' folder --> | ||
|
|
||
| <header> | ||
| <img src="img/karma-logo.svg" alt="Logo of the website" /> |
There was a problem hiding this comment.
| <img src="img/karma-logo.svg" alt="Logo of the website" /> | |
| <img src="img/karma-logo.svg" alt="Karma logo" /> |
We can make the alt description more descriptive
| <img src="img/icon-devices.svg" alt="Image | ||
| of different internet devices" /> |
There was a problem hiding this comment.
| <img src="img/icon-devices.svg" alt="Image | |
| of different internet devices" /> | |
| <img src="img/icon-devices.svg" alt="Different internet devices" /> |
We don't need to put "Image" in the alt description as we already know it's an image. Same goes for the other alt descriptions below
2 participants
GLASGOW_CLASS_5_OMER_ALI_HTML_CSS_WEEK_2
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?